-
Notifications
You must be signed in to change notification settings - Fork 438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #3241: Configuring the URI link target #3322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @IgorBaptist4! I tested this and confirmed it works. It's probably a good idea to add two tests for getLinkAttributes(urlValue), one with the environment.ui.baseUrl
and one without to make sure the right target is returned.
Good afternoon @nwoodward! Sorry for the delay in replying. As you suggested, I've added new tests to the test file. One checks if the getLinkAttributes method returns the correct attributes for internal links and the other checks if the getLinkAttributes method returns the correct attributes for external links. Any questions, I'm happy to help. |
Hi @Andrea-Guevara. Thank you for adding the tests! They look correct to me, and they pass when I run them. I'm not sure what the issue is with Codecov. Maybe someone with more knowledge about it could help us out. Thoughts, @tdonohue? |
@nwoodward and @Andrea-Guevara : The Codecov failure appears to be a false positive. Sometimes Codecov compares coverage percentages against the wrong prior commit (which seems to be the case here). Never been able to understand what causes it, but it occurs every now and then. Overall, this PR seems to have proper code coverage and Codecov is wrong. |
What a relief haha |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @IgorBaptist4 and @Andrea-Guevara!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks @IgorBaptist4 and @Andrea-Guevara ! The code here looks good to me. I've also realized that this same bug exists in 7.6.x, so I'll attempt to backport to both 8.x and 7.6.x
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin dspace-7_x
git worktree add -d .worktree/backport-3322-to-dspace-7_x origin/dspace-7_x
cd .worktree/backport-3322-to-dspace-7_x
git switch --create backport-3322-to-dspace-7_x
git cherry-pick -x 0ef2db146bc6c965e15582713e3a16ab4136c04b d7869f408ef221bc614547534a57b9b63a0aeef3 8572bfb1b1d1b053547047344e92bdae753ee44e |
Successfully created backport PR for |
Manually ported to 7.x in #3766 |
References
Note: To reproduce the error, use DSpace version 8.
Description
Creation of a new method in the “metadata-uri-values” component to identify when the url link on the item's simplified page is internal or external and where it will open.
Instructions for Reviewers
A getLinkAttributes method has been created which receives the value of the item's url and compares this value with the repository's base url; if the values of the urls are the same, the method returns the following attributes {target: '_self', rel:' '} and the link is opened in the same browser tab. Otherwise, the method returns the attributes {target: '_blank', rel: 'noopener noreferrer'} and the link is opened in a new browser tab. In the end, this conditional was dynamically added to the template via [attr.target] and [attr.rel].
List of changes in this PR:
To reproduce:
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You do not need to complete this checklist prior creating your PR (draft PRs are always welcome).
However, reviewers may request that you complete any actions in this list if you have not done so. If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
main
branch of code (unless it is a backport or is fixing an issue specific to an older branch).npm run lint
npm run check-circ-deps
)package.json
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.